-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes rust-windowing/winit#2597 for good #2747
Fixes rust-windowing/winit#2597 for good #2747
Conversation
Remove assert that is being randomly hit by a very small number of users
Hello, can someone give a quick review to this one? We need it to be able to use official winit instead of our fork! cc @madsmtm because you merged the previous one 🙏 |
I think removing the assert here is actually wrong, I can only come up with two reasons this fails:
So I would suggest the following:
|
If you know how to fix you can post a fix, it's just will be merged with delay. Though, if it's short and doesn't conflict with KB v2, we could merge. |
@Maximetinu could you do what I suggested above and figure out if this isn't an issue with the offset but just a racing condition? Unfortunately I don't seem to be able to reproduce this on my end. @kchibisov the fix shouldn't conflict with KB v2, I will make one as soon as we know more. |
Replaced by #2850. |
Hi @daxpedda , sorry to revive such an old thread, but, months later, I got a way to consistently reproduce this crash: #3579 Or, at least, it's a crash in what seems to be the same assert. When this happened to me months ago, it was caused by resizing the screen (but not always). As I say in this new #3579, the same assert can also be hit by opening the print dialog |
Remove assert that is being randomly hit by a very small number of users
This is a follow up to PR #2597. As explained in it (pls check it for context) there were a couple of asserts in this file that were unnecessary, and were causing some users to crash; while removing the assert works perfectly fine, so they don't seem to be preventing from anything serious.
In that PR, I was wary and changed the smallest amount of code possible (just removed 1 assert, out of 2). But now I've found some other users (still very few, maybe 0.001%) hitting the remaining assert. Now that time has verified that removing the first one didn't hurt and was the right call, I think that removing the remaining one is the way to go, for the same reasons explained in the previous PR, to prevent these crashes from happening anymore.